Feat: IoMmu protocol - #1728
Conversation
|
Thanks for working on this :) I haven't had a chance to look at the |
|
Is this ready for review? Currently it's marked as draft still. (No worries if that's intentional, just want to be sure you're not waiting on review.) |
|
I still need to write some test for it. I have been busy with some other things, so I'll send those over as soon as I can. Feel free to take a look if you want. |
|
@nicholasbishop I am trying to write some test for this, but I cant seem to get a protocol handle. I went into the uefi shell of the qemu vm, and the IoMmu driver image is present: dh -p 4e939de9-d948-4b0f-88ed-e6e1ce517c1e -v
Handle dump by protocol '4E939DE9-D948-4B0F-88ED-E6E1CE517C1E'
Shell> dh -p 03c4e603-ac28-11d3-9a2d-0090273fc14d -v
Handle dump by protocol 'PXEBaseCode'
Shell> dh 28 -v
28: 3ECF0C18
ImageDevicePath(3ECF2A98)
Fv(7CB8BDC9-F8EB-4F34-AAEA-3EE4AF6516A1)/FvFile(8657015B-EA43-440D-949A-AF3BE365C0FC)
LoadedImage(3ECF20C0)
Name..........: IoMmuDxeSo I am assuming the driver is failing to find hardware. I have tried a few different things, different EDK2 builds and followed the virtio instructions here, but nothing has worked. Ik it worked on hardware, so if you have any ideas on how to get qemu setup for IoMmu please let me know. |
Thanks for trying to find a solution! Is there any update here? Have you tried to add an IOMMU to the QEMU VM? Perhaps one is needed so that the protocol will be installed one some handles ( |
|
Any update here @PelleKrab ? If not, I'm in favor of closing the PR for now. |
No significant progress yet. I've been working on this here and there. Right now, I'm building a custom OVMF image to see if that forces the xtask VM to recognize the IOMMU drivers. I'm not sure if there’s wider interest in that approach, but I'm giving it a try since no other QEMU environment configs seem to help. I'm fine with closing the issue for now, and I can reopen it if I find a solution. |
Thanks! Good luck with your investigations. We are very happy for every contribution and feel free to reopen / open a new one as soon as you have a solution. For good project hygiene, we aim for short-living PRs (less than 3 months best) |
|
very cool! regarding ovmf-prebuilt I think @nicholasbishop is the expert - from my side it is looking good |
|
@phip1611 Everything is working now! Could you reopen this and take a look? |
|
Please rebase / solve the conflict and remove all |
|
The x86_64 CI jobs now require QEMU 9+ because they use |
|
thanks for working on that! @nicholasbishop any idea why this fails? |
|
Sorry, I lost focus and forgot to review. What is the status here? Do you still plan to upstream this? @PelleKrab |
All good, I believe it is the same issue with VM support for IOMMU on the CI OS version. I haven't looked at it recently, but I may be able to take a look at this weekend and see if anything has changed so that we can get the test to work in CI. |
|
I did some more testing in docker and in order to get the test passing we need a newer version of qemu (v10.1.0+), not necessarily a new Ubuntu version as per my last commit. Are there any dependencies or reasons stopping us from updating to a newer version? Also the current test failure seems to be some network timeout flakiness. |
This will unblock #1728. Also, it reverts #1884 which reverted #1876. We need the new version to unblock [0]. We however need to take care if the same CI failure causing #1884 occurs again or now. [0] #1728 (comment)
|
Interesting the invalid opcode failure is gone, but https test is failing. It used to fail on main locally, so I always assumed it was flaky. It appears to be some weird blocking, possibly requiring a bug fix on the OVMF. I have time this weekend to try to get this fixed. |
I think it would make sense to also bump ovmf. I can also try to investigate next week Thanks! |
|
I'm already looking into the OVMF bump FYI |
|
So it appears |
|
I am so happy that we can finally unblock this! In the last days, I've
I think if you rebase, we can finally proceed with a green CI 🥳 |
49aca86 to
1bc4146
Compare
|
Finally, all green!!!! Thank you for all the help. |
phip1611
left a comment
There was a problem hiding this comment.
Awesome - thanks for the patience!
I found at least two memory issues (assisted by Codex/GPT-5.5). These unit tests currently succeed but they should fail (or not even compile):
fn test_mapping_can_outlive_buffer(iommu: &Iommu) {
let attributes = EdkiiIommuAttribute::MEMORY_CACHED;
let mapping = {
let buffer = iommu
.allocate_buffer(MemoryType::BOOT_SERVICES_DATA, 1, attributes)
.expect("Failed to allocate IOMMU buffer");
let (_device_address, mapping, mapped_bytes) = iommu
.map(EdkiiIommuOperation::BUS_MASTER_READ, &buffer, buffer.size())
.expect("Failed to map IOMMU buffer");
assert_eq!(mapped_bytes, buffer.size());
mapping
};
drop(mapping);
}
fn test_map_accepts_oversized_length(iommu: &Iommu) {
let attributes = EdkiiIommuAttribute::MEMORY_CACHED;
let buffer = iommu
.allocate_buffer(MemoryType::BOOT_SERVICES_DATA, 1, attributes)
.expect("Failed to allocate IOMMU buffer");
let oversized_length = buffer.size() + 1;
let (_device_address, mapping, mapped_bytes) = iommu
.map(
EdkiiIommuOperation::BUS_MASTER_READ,
&buffer,
oversized_length,
)
.expect("IOMMU map accepted an oversized buffer length");
assert!(oversized_length > buffer.size());
assert_eq!(mapped_bytes, oversized_length);
drop(mapping);
}Problems:
-
Mapping is only tied to &Iommu, not to the DmaBuffer.
Safe code can drop/free the DMA buffer while the mapping is still alive, then unmap later. -
Iommu::map() accepts an arbitrary number_of_bytes.
Safe code can pass a length larger than the allocated DMA buffer, causing firmware to map memory outside the buffer.
Suggestions:
- Tie Mapping to the mapped buffer lifetime, for example Mapping<'iommu, 'buf> with PhantomData<&'buf DmaBuffer<'iommu>>, and make map() return a mapping that cannot outlive host_buffer.
- Remove the free-form length from the safe API, or validate it:
number_of_bytes <= host_buffer.size() should be enforced before calling firmware. A cleaner API would map the whole buffer by default and provide a checked subrange API if needed.
phip1611
left a comment
There was a problem hiding this comment.
Thanks, the lifetime issue is gone! I think there are a few more parts of the code that need further safety tightening.
| impl<'a> Deref for DmaBuffer<'a> { | ||
| type Target = [u8]; | ||
|
|
||
| fn deref(&self) -> &[u8] { |
There was a problem hiding this comment.
I am not 100% sure but I think we should replace this with
unsafe fn as_bytes(&self) -> &[u8] as the mapping might be mutuably in use while we are holding a shared reference to it. WDYT?
There was a problem hiding this comment.
I think you are 100% correct, there are scenarios where this memory can be written to or cleared by the system, so exposing a shared slice safely through Deref is too strong.
There was a problem hiding this comment.
On second thought, I think making map() take &mut DmaBuffer and tying Mapping to that mutable borrow should be enough here. The problematic case is safe Rust accessing the buffer while an active DMA mapping exists. If Mapping holds a PhantomData<&mut DmaBuffer>, then safe code cannot use Deref/DerefMut on the buffer until the mapping is dropped.
There was a problem hiding this comment.
Thanks for your work on this. A few more comments.
Also, please clean up the git commit history. Squash everything into three commits:
- uefi-raw: Derive more traits for IOMMU protocol
- uefi: add EDKII IOMMU protocol
- feat(test-runner): add IOMMU test coverage
|
Please fix CI. I think this is looking very good now! Thanks for all your patience! 🥳 Does this still work for you use-case, even with all the changes you've applied so far? |
|
I am no longer working on anything that needs this, but from what I can tell it should have all the functionality that my old team needs. Thank you for all the code review help! |
phip1611
left a comment
There was a problem hiding this comment.
Woop woop here comes the merge train 🚄 🚀
This PR adds the IoMmu Protocol to
uefi-rs, enabling easy DMA (Direct Memory Access) for devices. This is a draft to get some initial feedback before writinguefi-rstests. It is already working in my internal project. The main areas I would like feedback on are:unmap()andfree_buffer()?free_buffer_raw()andunmap_raw()be made public?General Implementation:
ref:
#1723